Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support compiling to wasm32 architectures #221

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Conversation

lulf
Copy link
Contributor

@lulf lulf commented Mar 1, 2023

Summary

Enable support for compiling sigstore-rs to WASM, making it usable from browser applications and on wasm32 VMs.

Example application using the Yew framework: https://github.com/trustification/sigstore-search

Closes #220

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's cool. Would it be possible to add an example of this kind of usage?

Cargo.toml Outdated
openidconnect = { version = "2.3", default-features = false, features = [ "reqwest" ], optional = true}
getrandom = { version = "0.2.8", features = ["js"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is something we have to do, quoting the documentation of the getrandom crate:

This feature should only be enabled for binary, test, or benchmark crates. Library crates should generally not enable this feature, leaving such a decision to users of their library. Also, libraries should not introduce their own js features just to enable getrandom’s js feature.

I think a better approach would be to:

  • Extend our documentation explaining how to use this crate when one of the WebAssembly targets is used
  • Move the getrandom dependency to the dev-dependencies. This will ensure the unit tests can be run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add a 'wasm' feature (and document it) to the crate that enabled it instead? I think it's a bit strange to require 'user' crates to set features on transient dependencies in order for things to compile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already ran into some crates that require that, so it doesn't feel so strange to me. However, I think adding a wasm feature would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've update to enable the js flag only if the wasm feature is enabled, and also added some info to the readme.

.github/workflows/tests.yml Show resolved Hide resolved
@lulf lulf force-pushed the wasm branch 2 times, most recently from d93bfb4 to 5344ae9 Compare March 2, 2023 13:49
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can be merged once all the tests are green

Enable support for compiling sigstore-rs to WASM, making it usable from
browser applications and on wasm32 VMs.

Signed-off-by: Ulf Lilleengen <lulf@redhat.com>
@lulf
Copy link
Contributor Author

lulf commented Mar 2, 2023

Clippy and integration tests compile now, and I squashed the commits. I think the GHA should be correct as well.

@flavio flavio merged commit cb011e5 into sigstore:main Mar 2, 2023
@lulf lulf deleted the wasm branch March 2, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support compiling on wasm32 architectures
2 participants